[improve][functions]Adding boolean in contextImpl.java to check partitioned topic present or not.#23407
[improve][functions]Adding boolean in contextImpl.java to check partitioned topic present or not.#23407nikam14 wants to merge 6 commits intoapache:masterfrom
Conversation
| .forEach(consumer -> topicConsumers.putIfAbsent(TopicName.get(consumer.getTopic()), consumer)); | ||
| .forEach(consumer -> { | ||
| topicConsumers.putIfAbsent(TopicName.get(consumer.getTopic()), consumer); | ||
| if (consumer.getTopic().contains("-partition-")) { |
There was a problem hiding this comment.
Please use the isPartitioned() in TopicName class to determine whether the topic is partitioned.
lhotari
left a comment
There was a problem hiding this comment.
Thanks for the contribution @nikam14. Please check the review comments. In addition, there would have to be a test case to cover this. One possible location for the test case could be one of the classes in https://github.com/apache/pulsar/tree/master/pulsar-broker/src/test/java/org/apache/pulsar/io directory so that there would be a end-to-end test to cover the use case. In addition, a unit test could be added to https://github.com/apache/pulsar/blob/master/pulsar-functions/instance/src/test/java/org/apache/pulsar/functions/instance/ContextImplTest.java if it is feasible.
@nikam14 Could you please provide more context about the specific use case where you encountered this issue? It would be helpful to understand:
This additional information will help us better understand the impact on end-users and ensure our solution addresses the root cause effectively. |
|
1 - With these changes when try to get |
Please explain the context of the end user use case. As a pulsar end user, what is the use case? Please provide an example function that shows the problem. |
|
changes are covered in |
Ok. What is the current error message without this change? |
this is not a end user test case. It's possible that you are making incorrect assumptions about the behavior as well as the reviewers. In Pulsar, the partitioned topics concepts can easily mislead anyone. Please see #20622 for more details. |
Motivation
In
contextImpl.javathegetConsumer()- When trying to get partitioned topic consumer it throws error consumer not found.Instead show error message that partitioned topic not present.
Also reduces computation to search for consumer.
Modifications
Added a boolean var to check partitioned topic present or not.
when try to get consumer check whether partitioned topic present then proceed.
Verifying this change
(Please pick either of the following options)
This change is already covered by existing tests, such as
ContextImplTest.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: